[SPARK-34484][SQL] Rename `map` to `mapAttr` in Catalyst DSL #31601

sarutak · 2021-02-20T15:10:33Z

What changes were proposed in this pull request?

This PR proposes to rename map to mapAttr in dsl/package.scala .
This PR also removes APIs which take Symbol type parameters.

Why are the changes needed?

With the Catalyst DSL (dsl/package.scala), we have two ways to represent attributes.

Symbol literals (' syntax)
$"" syntax which is defined in sql/catalyst module using string context.

But they have problems.

Regarding symbol literals, the scala community deprecates the symbol literals in Scala 2.13. We could alternatively use Symbol constructor but what is worse, Scala will completely remove Symbol in the future (https://scalacenter.github.io/scala-3-migration-guide/docs/incompatibilities/dropped-features.html).

Although scala.Symbol is useful for migration, beware that it is deprecated and that it will be removed from the scala-library. You are recommended, as a second step, to replace them with plain string literals "xwy" or a dedicated class.

Regarding $"" syntax, this has two problems.
The first problem is that the syntax conflicts with another $"" syntax defined in sql/core module.
You can easily see the problem with the Spark Shell.

import org.apache.spark.sql.catalyst.dsl.expressions._
val attr1 = $"attr1"

       error: type mismatch;
        found   : StringContext
        required: ?{def $: ?}
       Note that implicit conversions are not applicable because they are ambiguous:
        both method StringToColumn in class SQLImplicits of type (sc: StringContext): spark.implicits.StringToColumn
        and method StringToAttributeConversionHelper in trait ExpressionConversions of type (sc: StringContext): org.apache.spark.sql.catalyst.dsl.expressions.StringToAttributeConversionHelper
        are possible conversion functions from StringContext to ?{def $: ?}

The second problem is that we can't write like $"attr".map(StringType, StringType), though we can write 'attr.map(StringType, StringType).
This seems to be a bug of the Scala compiler and will be fixed in neither 2.12 nor 2.13 (scala/scala#7396).

Actually, I'm working on replacing all the symbol literals with $"" syntax in SPARK-34443 (#31569) and I found this problem in the following test code.

EncoderResolutionSuite.scala
ComplexTypeSuite.scala
ObjectExpressionsSuite.scala
NestedColumnAliasingSuite.scala
ReplaceNullWithFalseInPredicateSuite.scala
SimplifyCastsSuite.scala
SimplifyConditionalSuite.scala

[error] /home/kou/work/oss/spark-scala-2.13/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/EncoderResolutionSuite.scala:212:28: too many arguments (found 2, expected 1) for method map: (f: org.apache.spark.sql.catalyst.expressions.Expression => A): Seq[A]
[error]       $"a".map(StringType, StringType)).foreach { attr =>

As a solution for those problems, I propose to rename map to mapAttr.
With this solution, we can write "name".attr.mapAttr rather than $"name".map or 'name.map.

Does this PR introduce any user-facing change?

No. dsl/package.scala is for internal use.

How was this patch tested?

Modified the tests.

SparkQA · 2021-02-20T16:11:49Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39895/

SparkQA · 2021-02-20T16:30:10Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39895/

SparkQA · 2021-02-20T20:00:21Z

Test build #135315 has finished for PR 31601 at commit 5b9c205.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

sarutak · 2021-02-22T13:14:15Z

cc: @cloud-fan

cloud-fan · 2021-02-22T13:17:31Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/EncoderResolutionSuite.scala


    // int type can be up cast to long type
-    val attrs1 = Seq('a.string, 'b.int)
+    val attrs1 = Seq(attr("a").string, attr("b").int)


We already have "name".attr (See the implicit DslString), can we extend it to support "name".attr.string or even shorter "name".string?

Actually, it's not enough. "name".attr returns UnresolvedAttribute as well as $"name" so we still fail to do "name".attr.map.

My general idea is, previous the DSL is based on symbol, can we update the DSL (those implicts) so that it's based on string? Ideally we should simply replace 'abc to "abc" in the test code.

It was my first idea but I thought it might be confusable that "abc" with implicit conversion doesn't look like an attribute. But DSL is for internal so it might be acceptable. What do you think?

Ideally we should simply replace 'abc to "abc" in the test code.

Yes ideally it should be like what you suggested but I found it's difficult.
DslString implements ImplicitOperators but DslSymbol implements ImplicitAttribute. The implementation of expr conflicts. DslString implements expr as Literal but DslSymbol implements it as UnresolvedExpression.

So, the next option would be introduce a new syntax like attr"" with StringContext.

I said in the description like as follows.

Another solution would be to introduce a new string interpolation syntax but it's difficult to have workaround when the syntax is conflict with another string interpolation syntax like that $"" defined in catalyst and core conflict so I don't adopt this solution.

But if this is internal usage only, it might be acceptable.

SparkQA · 2021-02-22T15:42:05Z

Test build #135348 has finished for PR 31601 at commit c87915d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-02-22T16:21:09Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39929/

SparkQA · 2021-02-22T16:48:10Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39929/

SparkQA · 2021-02-22T17:23:50Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39931/

SparkQA · 2021-02-22T18:59:30Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39931/

sarutak · 2021-02-22T20:50:09Z

cc: @dongjoon-hyun and @HyukjinKwon too for other people's opinion.

SparkQA · 2021-02-22T21:12:05Z

Test build #135351 has finished for PR 31601 at commit 9f78964.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2021-02-23T03:03:00Z

I agree that the $"abc" is not great as it conflicts with the well-known syntax defined in sql/core. But I do like the existing "abc".attr syntax instead of the new one. Can we rename map to mapAttr to avoid the scala bug?

sarutak · 2021-02-24T08:28:00Z

Thanks for the advice. Renamed map to mapAttr. It seems to become not symmetric with struct and array but it can be acceptable as it's for internal use.
I also updated the description.

SparkQA · 2021-02-24T08:48:30Z

Test build #135412 has finished for PR 31601 at commit b306786.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-02-24T11:31:43Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40001/

SparkQA · 2021-02-24T12:04:33Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40001/

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala

cloud-fan

LGTM, please update the PR title

SparkQA · 2021-02-26T04:44:11Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40071/

SparkQA · 2021-02-26T05:00:39Z

Test build #135490 has finished for PR 31601 at commit a9e3490.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

sarutak · 2021-02-26T05:32:39Z

Newly added RemoveNoopUnionSuite seems to use symbol literal, which causes the build failure.
I'll fix it too.

SparkQA · 2021-02-26T06:41:18Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40073/

SparkQA · 2021-02-26T07:11:10Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40073/

SparkQA · 2021-02-26T10:29:43Z

Test build #135492 has finished for PR 31601 at commit e3dead4.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-02-26T18:40:38Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40097/

SparkQA · 2021-02-26T19:17:43Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40097/

SparkQA · 2021-02-26T22:36:32Z

Test build #135517 has finished for PR 31601 at commit 0af19a1.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
sealed trait PartitionSpec extends LeafExpression with Unevaluable
trait V2PartitionCommand extends Command
case class TruncateTable(table: LogicalPlan) extends Command
case class TruncatePartition(
case class TruncatePartitionExec(

sarutak · 2021-03-01T02:27:28Z

@cloud-fan According to this, should we wait for what happens as a result of the discussion?

cloud-fan · 2021-03-01T06:58:25Z

Is there a released Scala version that forbids the symbol's literal syntax?

sarutak · 2021-03-01T06:59:23Z

No, but we get warning with Scala 2.13.

EDIT: dotty seems not to support symbol literals.
https://scalacenter.github.io/scala-3-migration-guide/docs/incompatibilities/dropped-features.html

cloud-fan · 2021-03-01T07:09:19Z

OK seems the ship is already sailed. Let's wait a bit more but I'm not sure we can change that decision...

sarutak · 2021-03-01T07:18:17Z

OK seems the ship is already sailed. Let's wait a bit more but I'm not sure we can change that decision...

Yeah, I think so too. At least, symbol literals will be no longer supported as dotty already removed them.
https://dotty.epfl.ch/docs/reference/dropped-features/symlits.html

SparkQA · 2021-03-03T06:00:14Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40273/

SparkQA · 2021-03-03T06:34:33Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40273/

sarutak · 2021-03-18T04:37:39Z

@cloud-fan Surprisingly, Scala 3 seems to implement the backward compatibility feature for symbol literals (it may be a life extension measure, however).
scala/scala3#11588

So, I think replacing symbol literals with mapAttr might not be needed.
How about changing map to mapAttr? Do you think it's not needed too?
If so, I'll just close this PR.

cloud-fan · 2021-03-18T07:25:05Z

If scala will keep that backward compatibility feature forever, then we don't need to do anything. Otherwise, we still need to prepare for the removal of the symbol syntax.

sarutak · 2021-03-18T08:10:52Z

f scala will keep that backward compatibility feature forever,

According to this, it seems to be a life extension measure and will be removed in the future.
But I'm asking the Scala community.

github-actions · 2021-06-27T00:07:58Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

sarutak added 2 commits February 20, 2021 16:11

Introduce DslAttributeSymbol.

0888714

Modify tests to use attr() rather than ' syntax.

5b9c205

github-actions bot added the SQL label Feb 20, 2021

sarutak mentioned this pull request Feb 22, 2021

[SPARK-34443][CORE] Replace symbol literals with Symbol constructor invocations to comply with Scala 2.13 #31569

Closed

cloud-fan reviewed Feb 22, 2021

View reviewed changes

Use attr"" rather than attr()

c87915d

Fix test.

9f78964

sarutak mentioned this pull request Feb 22, 2021

[SPARK-34499][SQL] Improve the catalyst expression dsl internal APIs #31612

Closed

sarutak added 2 commits February 24, 2021 16:49

Merge branch 'master' of https://github.com/apache/spark into dsl-attr

826cb27

Rename map to mapAttr in dsl/package.scala.

b306786

sarutak changed the title ~~[SPARK-34484][SQL] Introduce a new syntax to represent attributes with the Catalyst DSL~~ [SPARK-34484][SQL] Introduce a new syntax to represent map types with the Catalyst DSL Feb 24, 2021

Fix style.

fb6041e

cloud-fan reviewed Feb 24, 2021

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala Show resolved Hide resolved

cloud-fan approved these changes Feb 24, 2021

View reviewed changes

Merge branch 'master' of https://github.com/apache/spark into dsl-attr

bf1573e

Replace symbol literals in RemoveNoopUnionSuite.

e3dead4

Resolve conflict.

0af19a1

sarutak force-pushed the dsl-attr branch 2 times, most recently from e03dca2 to 506bf5b Compare March 3, 2021 04:29

Resolve conflict.

cde070e

sarutak force-pushed the dsl-attr branch from 506bf5b to cde070e Compare March 3, 2021 04:32

cloud-fan mentioned this pull request Apr 29, 2021

[SPARK-35151][BUILD] Add compile arg to suppress symbol literal is deprecated compilation warnings in Scala 2.13 #32261

Closed

github-actions bot added the Stale label Jun 27, 2021

github-actions bot closed this Jun 28, 2021

[SPARK-34484][SQL] Rename map to mapAttr in Catalyst DSL #31601

[SPARK-34484][SQL] Rename map to mapAttr in Catalyst DSL #31601

Uh oh!

Conversation

sarutak commented Feb 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Feb 20, 2021

Uh oh!

SparkQA commented Feb 20, 2021

Uh oh!

SparkQA commented Feb 20, 2021

Uh oh!

sarutak commented Feb 22, 2021

Uh oh!

cloud-fan Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarutak Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Feb 22, 2021

Choose a reason for hiding this comment

Uh oh!

sarutak Feb 22, 2021

Choose a reason for hiding this comment

Uh oh!

sarutak Feb 22, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 22, 2021

Uh oh!

SparkQA commented Feb 22, 2021

Uh oh!

SparkQA commented Feb 22, 2021

Uh oh!

SparkQA commented Feb 22, 2021

Uh oh!

SparkQA commented Feb 22, 2021

Uh oh!

sarutak commented Feb 22, 2021

Uh oh!

SparkQA commented Feb 22, 2021

Uh oh!

cloud-fan commented Feb 23, 2021

Uh oh!

sarutak commented Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Feb 24, 2021

Uh oh!

SparkQA commented Feb 24, 2021

Uh oh!

SparkQA commented Feb 24, 2021

Uh oh!

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 26, 2021

Uh oh!

SparkQA commented Feb 26, 2021

Uh oh!

sarutak commented Feb 26, 2021

Uh oh!

SparkQA commented Feb 26, 2021

Uh oh!

SparkQA commented Feb 26, 2021

Uh oh!

SparkQA commented Feb 26, 2021

Uh oh!

SparkQA commented Feb 26, 2021

Uh oh!

SparkQA commented Feb 26, 2021

[SPARK-34484][SQL] Rename `map` to `mapAttr` in Catalyst DSL #31601

[SPARK-34484][SQL] Rename `map` to `mapAttr` in Catalyst DSL #31601

sarutak commented Feb 20, 2021 •

edited

Loading

cloud-fan Feb 22, 2021 •

edited

Loading

sarutak Feb 22, 2021 •

edited

Loading

sarutak commented Feb 24, 2021 •

edited

Loading

sarutak commented Mar 1, 2021 •

edited

Loading

sarutak commented Mar 1, 2021 •

edited

Loading

cloud-fan commented Mar 18, 2021 •

edited

Loading